Skip to content

Issue 63: Upgrade to aws-sdk-java-v2 #83

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 20 commits into from
Nov 5, 2020
Merged

Issue 63: Upgrade to aws-sdk-java-v2 #83

merged 20 commits into from
Nov 5, 2020

Conversation

@ptirador
Copy link
Contributor

@ptirador ptirador commented Sep 10, 2020 •
edited
Loading

Pull Request Description

This pull request closes #63 .

Acceptance Test

  • Building the code with mvn clean install -Pintegration-tests still works.

Questions

  • Does this pull request break backward compatibility?

    • Yes
    • No

  • Does this pull request require other pull requests to be merged first?

    • Yes, please see #...
    • No

  • Does this require an update of the documentation?

    • Yes, please see [provide details here]
    • No

Test classes:

org.carlspring.cloud.storage.s3fs

  • AttributesUtilsTest
  • CacheTest
  • ExampleClass
  • ExampleClassIT
  • FilesIT
  • FileSystemsIT
  • S3ClientFactoryTest
  • S3ClientIT
  • S3FileAttributesTest
  • S3FileChannelTest
  • S3FileStoreTest
  • S3FileSystemTest
  • S3IteratorTest
  • S3SeekableByteChannelTest
  • S3UnitTestBase
  • S3UtilsIT
  • S3UtilsTest
  • S3WalkerTest

org.carlspring.cloud.storage.s3fs.fileSystemProvider:

  • CheckAccessTest
  • CopyTest
  • CreateDirectoryTest
  • DeleteTest
  • GetFileAttributeViewTest
  • GetFileStoreTest
  • GetFileSystemIT
  • GetPathTest
  • IsHiddenTest
  • IsSameFileTest
  • MoveTest
  • NewByteChannelIT
  • NewByteChannelTest
  • NewDirectoryStreamTest
  • NewFileSystemIT
  • NewFileSystemTest
  • NewInputStreamTest
  • NewOutputStreamTest
  • ReadAttribuTestest
  • SetAttributeTest

org.carlspring.cloud.storage.s3fs.path:

  • EndsWithTest
  • EqualsTest
  • GetFileNameTest
  • GetKeyTest
  • GetNameTest
  • GetRootTest
  • IteratorTest
  • ResolveSiblingTest
  • ResolveTest
  • S3PathTest
  • StartsWithTest
  • SubpathTest
  • ToUriTest
  • ToURLIT

org.carlspring.cloud.storage.s3fs.spike

  • EnvironmentIT
  • FileStoreTest
  • InstallProviderTest
  • PathSpecTest
  • ProviderSpecTest
  • SpecTest
  • URISpikeTest

org.carlspring.cloud.storage.s3fs.util:

  • BrokenS3Factory
  • CopyDirVisitor
  • EnvironmentBuilder
  • ExposingS3Client
  • ExposingS3ClientFactory
  • FileAttributeBuilder
  • MockBucket
  • S3ClientMock
  • S3EndpointConstant
  • S3MockFactory
  • UnsupportedFileStoreAttributeView

carlspring
carlspring reviewed Sep 11, 2020
View reviewed changes
carlspring
carlspring reviewed Sep 11, 2020
View reviewed changes
@carlspring
Copy link
Owner

carlspring commented Sep 11, 2020 •
edited
Loading

@ptirado : This pull request might require re-working the examples in the wiki site that we're putting together in #22 / #70 .

@ptirador ptirador changed the title Issue-63: update main code to SDK v2. Issue-63: Upgrade to aws-sdk-java-v2 Sep 12, 2020
@ptirador ptirador force-pushed the issue-63 branch 2 times, most recently from 5167c30 to 24ae70e Compare September 12, 2020 17:34
@carlspring
Copy link
Owner

carlspring commented Sep 12, 2020

@ptirador : This page will also need to be updated once the documentation pull request is merged: docs/content/reference/configuration-options.md.

@steve-todorov
Copy link
Collaborator

steve-todorov commented Sep 12, 2020

Nice work @ptirador! :)
The new aws sdk v2 seems to provide async non-blocking client. Maybe we can also switch the nio implementation to use AsynchronousFileChannel? This should give a significant performance boost IMO. @carlspring maybe we should add this as a follow-up task? :)

@carlspring
Copy link
Owner

carlspring commented Sep 12, 2020

Nice work @ptirador! :)
The new aws sdk v2 seems to provide async non-blocking client. Maybe we can also switch the nio implementation to use AsynchronousFileChannel? This should give a significant performance boost IMO. @carlspring maybe we should add this as a follow-up task? :)

Yeah, we should investigate how we can benefit from the AsynchronousFileChannel. It would probably make more sense to add this in a follow-up task, because the migration to the new SDK is quite a bit of a change on its own, but, if @ptirador is up for it as part of this pull request, I'm fine with that as well. :)

@steve-todorov
Copy link
Collaborator

steve-todorov commented Sep 12, 2020

P.S. I've just merged some build improvements in the master branch. Please rebase to have faster build (including win and mac checks) :)

@ptirador
Copy link
Contributor Author

ptirador commented Sep 13, 2020

Nice work @ptirador! :)
The new aws sdk v2 seems to provide async non-blocking client. Maybe we can also switch the nio implementation to use AsynchronousFileChannel? This should give a significant performance boost IMO. @carlspring maybe we should add this as a follow-up task? :)

Yeah, we should investigate how we can benefit from the AsynchronousFileChannel. It would probably make more sense to add this in a follow-up task, because the migration to the new SDK is quite a bit of a change on its own, but, if @ptirador is up for it as part of this pull request, I'm fine with that as well. :)

I agree. This should be created in a follow-up task.

@ptirador
Copy link
Contributor Author

ptirador commented Sep 13, 2020

P.S. I've just merged some build improvements in the master branch. Please rebase to have faster build (including win and mac checks) :)

Rebase done 👍

@ptirador ptirador force-pushed the issue-63 branch 4 times, most recently from 4d01965 to 314dae5 Compare September 13, 2020 16:32
carlspring
carlspring reviewed Sep 15, 2020
View reviewed changes
carlspring
carlspring reviewed Sep 15, 2020
View reviewed changes
src/main/java/org/carlspring/cloud/storage/s3fs/S3FileStore.java Outdated
Comment on lines +134 to +145
Copy link
Owner

@carlspring carlspring Sep 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these notes coming from @elerch 's fork?

Copy link
Contributor Author

@ptirador ptirador Sep 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, some parts are from this fork that you mention. Could be helpful.

Copy link

@markjschreiber markjschreiber Oct 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ability to head the bucket is the true test that a bucket "exists" for the Principle as listing buckets only shows those that are in the account of the user whereas "head" will work if the bucket has been shared with the Principle from another account.

@ptirador ptirador force-pushed the issue-63 branch 3 times, most recently from ba80ea6 to fd91fc2 Compare September 21, 2020 15:55
@ptirador ptirador force-pushed the issue-63 branch 3 times, most recently from b741167 to 92ea6db Compare September 28, 2020 17:26
@ptirador
Issue-63: Socket Buffer Size Hint not supported in v2.
7bf910e 
fixup! Issue-63: Socket Buffer Size Hint not supported in v2.
@ptirador
Issue-63: Fix tests.
1a65049 
fixup! Issue-63: Fix tests.
@ptirador
Issue-63: Fix S3Factory to obtain correctly region and protocol, and …
3c99dd1 
…obtain rest of properties missed.
@ptirador
Issue-63: Revert replace usages to original replaceAll usages.
62c9bba
@ptirador
Issue-63: Refactor to declare S3Client in new line.
9200b5c
@ptirador
Issue-63: Add boolean parameter to use or not a temporary file in the…
0951348 
… fileChannel/seekableByteChannel creation.

fixup! Issue-63: Add boolean parameter to use or not a temporary file in the fileChannel/seekableByteChannel creation.
@ptirador
Issue-63: Handle integer and long parsing, and add radix.
830c1c9
@ptirador
Issue-63: Remove not valid case for URI.
c379e9a 
fixup! Issue-63: Remove not valid case for URI.
@ptirador
Copy link
Contributor Author

ptirador commented Oct 30, 2020

Nice work @ptirador! :)
The new aws sdk v2 seems to provide async non-blocking client. Maybe we can also switch the nio implementation to use AsynchronousFileChannel? This should give a significant performance boost IMO. @carlspring maybe we should add this as a follow-up task? :)

@carlspring, @steve-todorov : Follow-up task created: #99

@ptirador ptirador mentioned this pull request Oct 30, 2020
Closed
5 tasks
@carlspring
Copy link
Owner

carlspring commented Oct 30, 2020

@ptirador ,

Thanks for your massive work on this pull request! As agreed, I will merge this into the master.

@elerch, @markjschreiber ,

Thanks for reviewing and for all your comments! We've reviewed them and have decided to handle them as separate follow-up tasks. Your guidance has been extremely valuable and much appreciated and we hope you can keep an eye on our future pull requests as well!

@ptirador ptirador mentioned this pull request Oct 31, 2020
Open
1 task
carlspring
carlspring reviewed Nov 4, 2020
View reviewed changes
carlspring
carlspring reviewed Nov 4, 2020
View reviewed changes
carlspring
carlspring reviewed Nov 4, 2020
View reviewed changes
@ptirador
Copy link
Contributor Author

ptirador commented Nov 5, 2020

@carlspring: Your suggestions have been fixed.

@carlspring carlspring merged commit 215449e into carlspring:master Nov 5, 2020
@carlspring
Copy link
Owner

carlspring commented Nov 5, 2020

Awesome work, @ptirador !

Thanks again! :)

@carlspring carlspring mentioned this pull request Dec 9, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@carlspring carlspring carlspring left review comments

@steve-todorov steve-todorov steve-todorov left review comments

+2 more reviewers

@elerch elerch elerch left review comments

@markjschreiber markjschreiber markjschreiber left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Upgrade to aws-sdk-java-v2

5 participants

@ptirador @carlspring @steve-todorov @elerch @markjschreiber